[fix][client] PIP-486: explicit unsubscribe for scalable consumers' clean close - #26433
Conversation
A scalable consumer's registration lives on the controller, but its connection comes from the shared connection pool: a consumer's close never makes the channel inactive while the client process lives, so the controller never noticed a clean departure — the registration stayed "connected" indefinitely (a same-process leave was a permanent zombie group member) and even a cross-process clean leave stalled the rebalance for the full disconnect grace period. Adds CommandScalableTopicUnsubscribe (BaseCommand type 82): on close the client tells the controller to delete the registration and rebalance the group immediately; the disconnect grace period remains the fallback for unclean departures. - Client: the unsubscribe is chained on the in-flight subscribe attempt's outcome, so it can never overtake the registration (the broker records the per-connection registration ref before sending the subscribe response); best-effort — a failure never fails the close. - Broker: the handler resolves the consumer through the connection's own registration map (so a client can only unregister sessions it created), forwards to the existing SubscriptionCoordinator.unregisterConsumer (grace-timer cancel + persisted-entry delete + rebalance), and answers CommandSuccess, idempotently for unknown ids. The registration ref is removed only after the unregister succeeds, keeping the channelInactive grace fallback alive if the explicit path fails. The rejoin e2e now runs against the default grace period with all consumers on one shared client: the leave hands the segment back within seconds purely through the unsubscribe path (previously it required a dedicated client per leaver and a shrunken grace period). Assisted-by: Claude Code (Fable 5)
…sumer-unsubscribe
Retracted: posted in error by a local tooling test.
…sumer-unsubscribe
…iew findings Addresses lhotari's review on the PR: 1. Failed metadata delete no longer wedges the group: the coordinator's unregisterConsumer now deletes the persisted registration first and only on success removes the in-memory session, cancels its grace timer, and rebalances. A failure changes nothing — the session stays registered and connected, so the channelInactive → grace fallback the ServerCnx error path relies on actually works, and a retried unsubscribe retries the deletion instead of short-circuiting on an already-removed session. 2. close() can no longer be raced by overlapping reconnect attempts: a connection drop used to schedule two reconnects (failed pending request + connectionClosed), and close() chained its unsubscribe on a single last-attempt slot that either could overwrite — letting the older attempt register after the unsubscribe. Reconnect scheduling is now collapsed to one pending attempt, and close() chains on every in-flight subscribe attempt. 3. close() deregisters the local session immediately and unconditionally again: deferring removeScalableConsumerSession behind the subscribe future meant an unanswered subscribe retained the closed session (and pinned the pooled connection non-idle) for the connection's lifetime. Only the wire command waits for the in-flight attempts now. 4. Both scalable session requests (subscribe and unsubscribe) go through a new ClientCnx.sendScalableSessionRequest helper with the standard bookkeeping — request-timeout tracking and write-failure completion — instead of hand-rolled pendingRequests entries that never timed out and, on write failure, left the future forever pending. 5. Direct coverage for the command: a Commands round-trip test, a ServerCnx test for the idempotency contract and the error path (error response keeps the registration ref, a retry re-runs the unregister, success removes the ref), and a subscribe/close churn e2e asserting the group converges back to a sole Exclusive owner under the default grace period. The rejoin e2e's leaver is now tracked for cleanup so an early assertion failure cannot leak it onto the shared client. 6. ScalableTopicService.unregisterConsumer javadoc: the no-op branch is reached by deleted topics / shutdown / never-registered consumers — not by moved leadership, which keeps its controller entry and fails via checkLeader() into the ref-preserving error path. Assisted-by: Claude Code (Fable 5)
lhotari
left a comment
There was a problem hiding this comment.
All six points from the last round are addressed in 7ac6b40, and in each case by a change to the mechanism rather than to the comment describing it: the coordinator now deletes the persisted registration before it touches anything in memory, close() chains on every in-flight subscribe attempt instead of a single overwritable slot, the local session deregistration is immediate and unconditional again, and both scalable-session requests go through ClientCnx's standard request bookkeeping. The three new tests pin the parts that were untested: the command's wire round-trip, the broker's idempotency and error/retry contract, and a subscribe/close churn e2e. Details in each thread.
Two points on the new code, both narrower than anything in the last round:
- The request timeout that now covers the subscribe (a good change on its own) also gives the close-ordering invariant a way out: a subscribe can complete exceptionally while the connection is alive and the broker registration is still in flight, and the broker records its registration ref only after
registerConsumerresolves — so the unsubscribe that follows takes the "unknown consumer id" branch and the registration lands behind it. Raised onServerCnx.java:1192. - The delete-first reorder in the coordinator leaves one window where a same-name rejoin during the in-flight delete is removed by the delete's own continuation. Raised on
SubscriptionCoordinator.java:232.
Neither is a regression of the paths this PR set out to fix, and both have cheap containments; whether they belong here or in a follow-up is your call.
|
Feel free to mark the unresolved threads as resolved so that this PR can be merged. No blockers. |
…sumer-unsubscribe
…ations Two follow-up findings from review: 1. The unsubscribe could overtake a registration still in flight: the per-connection ref was recorded only after registerConsumer resolved, and since the subscribe now carries a request timeout, the client can give up (and close) while the broker-side registration is still running — the unsubscribe then found no ref, answered idempotent success, and the registration completed afterwards as a zombie. The ref is now recorded before the registration resolves and carries the registration future; the unsubscribe handler and the channelInactive sweep both chain behind it, so ordering no longer depends on how long the client was able to wait. A failed registration drops the ref. Both consumer builders also close the session when the subscribe fails, so an abandoned attempt still sends its unsubscribe once the in-flight registration settles. 2. The delete-first unregister left one window on the success path: a same-name rejoin during the in-flight delete takes the reconnect branch (attaching a new consumer id), and the delete's continuation then removed that very session and left the persisted entry erased. The removal is now identity-guarded by the leaving consumer's id (plumbed down from the connection's registration map): a stale-id leave keeps the rejoined session and restores the persisted registration the delete erased. Tests: a ServerCnx test pinning the ordering (an unsubscribe sent during an in-flight registration answers nothing until the registration completes, then unregisters); a coordinator identity test (stale-id leave keeps the rejoined session, live-id leave removes it); and a same-name close/resubscribe churn leg in the e2e. Assisted-by: Claude Code (Fable 5)
Motivation
Follow-up promised in #26208: a scalable consumer's registration lives on the controller, but the controller connection comes from the shared connection pool — so a consumer's clean close never makes a channel inactive while the client process lives, and the controller never noticed the departure. A same-process leave (several consumers in one process is the common case) left a zombie group member indefinitely: the disconnect grace timer never even started, the group never rebalanced, and the leaver's segment was never handed back. Even a cross-process clean leave stalled the rebalance for the full grace period (default 60s).
Modifications
Wire format
CommandScalableTopicUnsubscribe { request_id, consumer_id }(BaseCommandtype 82): a scalable consumer cleanly leaving its subscription. Acknowledged with the standardCommandSuccess/CommandError; idempotent — an unknownconsumer_id(e.g. already swept by a disconnect) still succeeds. No new response message.Broker
ServerCnx.handleCommandScalableTopicUnsubscribe: resolves the consumer through the connection's own registration map, so a client can only unregister sessions it created on that connection — no separate authorization needed. Forwards to the existingSubscriptionCoordinator.unregisterConsumer(cancels the grace timer, deletes the persisted registration, rebalances and notifies the remaining consumers) via a newScalableTopicServicepassthrough.channelInactivesweep can still report the disconnect later, so the grace-period fallback stays alive.Client
ScalableConsumerClient.close()sends the unsubscribe, chained on the in-flight subscribe attempt's outcome: the broker records the registration ref before sending the subscribe response, so the unsubscribe can never overtake its own registration (an early unsubscribe would no-op and leave a ghost behind). Best-effort throughout — a failure never fails the close; the grace period remains the fallback for unclean departures.The disconnect/grace machinery is unchanged and still covers crashes and connection loss.
Verifications
testConsumerRejoiningAfterLeaveDoesNotWedgeReleasenow runs with all consumers on one shared client against the default 60s grace period: the leave hands the whole segment back within seconds purely through the unsubscribe path (previously the test needed a dedicated client per leaver plus a shrunken grace period to see a departure at all).